Repository navigation
Css keywords - #775
Css keywords#775AlexanderWhitestone wants to merge 1 commit into
Conversation
aaltat
left a comment
aaltat
left a comment
There was a problem hiding this comment.
Fix the comments and write tests. At least write write following tests:
- Write few test that verify happy path
- Test where property does not contain expected value
- Test where element does not contain the CSS property
- Test where css property is a number. Considering also RF providing the number in different types: 5 versus ${5}
- Test where CSS property contains multiple elements, like: https://www.w3schools.com/cssref/pr_border.asp
| ## ## | ||
| ## Author: Olivier Verville ## | ||
| ## Date: May 24th 2016 ## | ||
| ###################################################################### |
| ###################################################################### | ||
| def element_css_attribute_should_be(self, locator, prop, expected): | ||
| """Verifies the element identified by `locator` has the expected | ||
| value for the targeted `prop`. |
There was a problem hiding this comment.
Doc strings must follow pep256
| return element.text | ||
| return None | ||
|
|
||
| def _check_element_css_value(self, locator, prop, expected): |
There was a problem hiding this comment.
Because this method is only used in the Element Css Attribute Should Be it might be better idea to move the code from here to the keyword.
|
|
||
| element = self._element_find(locator, True, True) | ||
|
|
||
| value = element.value_of_css_property(prop) |
There was a problem hiding this comment.
Following pep8 is required, specially on the line length.
| element = self._element_find(locator, True, True) | ||
|
|
||
| value = element.value_of_css_property(prop) | ||
| if (value != expected): |
There was a problem hiding this comment.
Parentheses are not needed.
|
|
||
| value = element.value_of_css_property(prop) | ||
| if (value != expected): | ||
| raise AssertionError("Element locator '%s' css property '%s' had a "\ |
There was a problem hiding this comment.
ValueError could be better error class.
|
I agree with all the requested changes. Thanks @aaltat for the review and the feedback. |
|
For the new keyword, the code change looks OK. But it is missing the tests, which is not OK. Also it looks like you used a tool to fix pep8 related errors, which is good thing too. But one pull request should do only a one thing. Having multiple fixes/enhancement in a one pull request makes the review harder. You can leave it for now but could you remember in the future. |
|
I agree, I'll keep that in mind. |
|
It'sexplained in here: https://github.com/robotframework/Selenium2Library/blob/master/CONTRIBUTING.md#code-contributions and in here: https://github.com/robotframework/Selenium2Library/blob/master/BUILD.rst#unit-and-acceptance-tests If you are developing in Windows, it might be good idea rebase with master. There is commit in master which fixes the test server problems when running in Windows. |
|
How is the test case writing going on? |
|
I'm unstuck, but I haven't had time to work on them. Work, school, competitions, and consulting have had me fully booked. |
|
How is the test case writing going on? |
|
I haven't had time to work on them directly, but I have written some tests for other new keywords not yet pulled. So the learning part is done. I'm still planning on adding the tests, I just don't know when it will happen. |
|
I discovered some limitations. |
|
It could also be the browser transforming color names to RGB values. In that case the actual behavior could also depend on the browser. |
|
Verifying CSS can be complicated and one can run into unexpected problems. But I think it's still a good keyword to have. The next release is coming soonnish and it would be nice to get this in. |
|
I'm about half done with the tests. If the color values are browser specific, then I think that will have to be handled by the user of the keyword. For now I won't test for colors values because it might require browser specific conditions and would be difficult to maintain. |
|
Sounds good to me. |
|
Have you found time to finish this PR? |
|
Sorry for the delay. I just started a new job so I haven't been working on this. |
|
No worries. Do you think that at some point you would have time to finish this PR? |
|
Yes, I will have time at some point. |
|
Do we still want this? I let this PR get really old and I want to know if i should try fixing it. |
|
Welcome back after so long time. The feature gets asked now and then in the community and therefore the keywords would be good addition to the library. And if I recall correctly, it was just missing few acceptance tests. If you have time and will to fix the PR, I am happy to review and merge it. The history should be cleaned tough. We usually do not use merge commit's and history should be clean. Which means that your commits should be on top of the commit's that are already in the master. |
|
Yea I messed up the history while trying to pull in the latest. I will work on it and do a proper rebase so the commits are on top. |
|
No problems, take your time. |
|
Are you still planning to work on the PR or can we close it? |
|
We can close it. I haven't been using the library and I'm not long familiar with it unfortunately. I fixed the history and pushed what I had incase it helps anyone coming in to work on this in the future. |
9a0e122 to
390a709
Compare
|
I understand and now worries. Thanks for effort and if there is a need, it can be always found from the GitHub. |
This keyword was written in a fork (I didn't write it)
Issue #774
I wasn't sure if I should keep the comment about the author or not.
I kept it because it was in the original repo.